Pull request overview

Adds a configurable delay between retry attempts for the retry-failed-tests extension to reduce flakiness for external-system-dependent tests.

Changes:

• Introduces --retry-failed-tests-delay CLI option with unit-aware timespan parsing and validation.
• Applies an inter-attempt Task.Delay in RetryOrchestrator and strips the option from forwarded child-process args.
• Updates help text, package docs, resources, and adds unit tests for option validation.

Copilot's findings

• Files reviewed: 20/20 changed files
• Comments generated: 4

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

1. [Critical — will cause test failure] HelpInfoAllExtensionsTests.cs: The --help output test was updated for --retry-failed-tests-delay, but the Info_WithAllExtensionsRegistered_OutputFullInfoContent test was not. Because AssertOutputMatchesLines performs sequential line-by-line matching, the test will fail in CI — the actual --info output now includes the delay option under RetryCommandLineOptionsProvider, but the expected pattern jumps straight to TerminalTestReporterCommandLineOptionsProvider. Four lines need to be inserted in the pattern.

2. [Minor — coverage gap] RetryTests.cs (IsInvalid_If_InvalidTimeSpan_Is_Provided_For_DelayOption): The whitespace-only input (" ") takes a distinct early-return path in TimeSpanParser.TryParse but is not included in the [DataRow] set.

3. [Moderate — missing behavioral coverage] Neither the new unit tests nor the existing RetryFailedTestsTests.cs integration tests verify the actual delay behaviour in RetryOrchestrator: that Task.Delay is applied between retry attempts (not before the first), and that --retry-failed-tests-delay is stripped from the child-process argument list.

Recommendations

• Add the --retry-failed-tests-delay block to the --info expected-output string in HelpInfoAllExtensionsTests.cs (around line 342, after the --retry-failed-tests-max-tests entry).
• Add [DataRow(" ")] to IsInvalid_If_InvalidTimeSpan_Is_Provided_For_DelayOption.
• Add an acceptance or unit test that exercises the orchestrator's delay and arg-stripping logic end-to-end.

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

[Defensive] TimeSpanParser.Parse in orchestrator path (RetryOrchestrator.cs:158)
The orchestrator calls TimeSpanParser.Parse (which throws FormatException) rather than the TryParse pattern used elsewhere in the same method. No bounds check on retryDelayArgs before [0] access either, inconsistent with how cmdRetries is guarded at line 98. Low practical risk since validation runs first, but adds a latent uncaught-exception path.

[Resources] Pipe server created before delay fires (RetryOrchestrator.cs:181 / 282)
RetryFailedTestsPipeServer (OS named-pipe listener) is created ~100 lines before the Task.Delay fires. For large delays (minutes/hours), the pipe holds OS resources for the entire wait period before the child process is launched. Moving the delay to the top of the loop body (immediately after attemptCount++) would scope the pipe server to only the window where it's needed.

Positive Observations

• Correctness of the retry-delay logic is sound: delay correctly skips attempt 1 (attemptCount > 1), fires before each subsequent attempt, and applies ConfigureAwait(false) throughout.
• Cancellation token is correctly threaded into Task.Delay — cancellation during the wait will propagate as OperationCanceledException, which is the expected behavior for this framework.
• Argument stripping (indexToCleanup) correctly removes both the option flag and its value, consistent with all other stripped options.
• Validation in RetryCommandLineOptionsProvider is complete: dependency on --retry-failed-tests, format validation via TryParse, and test coverage for valid/invalid inputs.
• TimeSpanParser.TryParse correctly rejects empty strings and invalid formats per the new unit tests.

Overall Assessment

The implementation is logically correct and well-structured. Both findings are minor defensive/resource concerns rather than correctness bugs. No threading issues, security vulnerabilities, or public API breaking changes were identified.

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

Copilot's findings

• Files reviewed: 21/21 changed files
• Comments generated: 1

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.